-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade es and kb to version 8 #3598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some UT failures need to be addressed.
"SYS_CHROOT", | ||
} | ||
// Set the user and group to be the default elasticsearch ID | ||
sc.RunAsUser = ptr.Int64ToPtr(1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the init containers still need to run as root to:
- set os settings
- chown the file system
and so the namespace still needs to have the privileged context. The main container now runs as 1000:1000, which is the es user. This is also the user the prepareFS script will chown to.
Is my understanding correct? Have we considered changing the user to 10000:10000 (our default user)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this SecurityContext is for the main ElasticSearch container. The SecurityContext for each init container still uses the root context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Init container "elastic-internal-init-filesystem" still run as root because we need to set the vm max map count "echo 262144 > /proc/sys/vm/max_map_count". This change is only for elasticsearch container.
Currently, Elasticsearch 7, we override the security context of the Elasticsearch container to run as the root user and group. The docker-entrypoint.sh script in Elasticsearch contained logic to switch to user 1000 when it encountered a root user. However, this logic has been removed in Elasticsearch 8. Since the ownership of the /data folder is set to the default Elasticsearch UID, we are encountering an unwritable error when running as the root user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We spoke offline about a potential follow-up to explore using uid:gid of 10001:10001, but continue with the PR in its current form.
"data", | ||
"ingest", | ||
"master", | ||
"remote_cluster_client", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need these role - data, ingest, master same as the previous version.
Reg:remote_cluster_client : if we are setting node.roles, then remote_cluster_client role must be explicitly included to enable cross-cluster search. Otherwise, this could result in an access denied issue when loading the Stack Monitoring page in Kibana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:/ so be it...
pkg/render/logstorage.go
Outdated
@@ -359,14 +360,9 @@ func (es *elasticsearchComponent) podTemplate() corev1.PodTemplateSpec { | |||
} | |||
|
|||
sc := securitycontext.NewRootContext(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this securitycontext is for the main ElasticSearch container. It should use securitycontext.NewNonRootContext()
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
58b1252
to
af72eab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
https://github.com/tigera/calico-private/pull/8119
Description
For PR author
make gen-files
make gen-versions
For PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bug
if this is a bugfix.kind/enhancement
if this is a a new feature.enterprise
if this PR applies to Calico Enterprise only.